Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: masks ip address in request details passed as argument to MN client #3268

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: masks ip address in request details passed as argument to MN client #3268

wants to merge 2 commits into from
+49 −34

Conversation

konstantinabl
Copy link
Collaborator

@konstantinabl konstantinabl commented Nov 15, 2024 •
edited
Loading

Description:

The issue arises from the fact requestDetails are passed as an argument to the repeatedRequest method in MN client, because they are needed for the next method call

public async repeatedRequest(methodName: string, args: any[], repeatCount: number, requestDetails?: RequestDetails) {
    let result;
    for (let i = 0; i < repeatCount; i++) {
      try {
        result = await this[methodName](...args);
      } catch (e: any) {
      .......

However, further down in the method the args are logged.

Related issue(s):

Fixes #3266

@konstantinabl konstantinabl linked an issue Nov 15, 2024 that may be closed by this pull request
Relay is logging IP addresses #3266
Open
@konstantinabl konstantinabl self-assigned this Nov 15, 2024
@konstantinabl konstantinabl added the Internal For changes that affect the project's internal workings but not its outward-facing functionality. label Nov 15, 2024
@konstantinabl konstantinabl added this to the 0.61.0 milestone Nov 15, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 15, 2024 •
edited
Loading

Test Results

 21 files   - 1  281 suites   - 10   35m 1s ⏱️ -41s
609 tests ±0  603 ✅ + 1  4 💤 ±0  2 ❌  - 1 
705 runs   - 3  697 ✅  -  2  4 💤 ±0  4 ❌  - 1 

For more details on these failures, see this check.

Results for commit 7e24f26. ± Comparison against base commit b99655e.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both. 
"before all" hook in "@api-batch-2 RPC Server Acceptance Tests" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before all" hook in "@api-batch-2 RPC Server Acceptance Tests"

"before all" hook for "should associate to a token" ‑ RPC Server Acceptance Tests Acceptance tests @tokencreate HTS Precompile Token Create Acceptance Tests "before all" hook for "should associate to a token"

♻️ This comment has been updated with latest results.

Nana-EC
Nana-EC reviewed Nov 15, 2024
View reviewed changes
packages/relay/src/lib/eth.ts Outdated
@@ -1722,7 +1722,7 @@ export class EthImpl implements Eth {
const formattedTransactionId = formatTransactionIdWithoutQueryParams(submittedTransactionId);
const contractResult = await this.mirrorNodeClient.repeatedRequest(
this.mirrorNodeClient.getContractResult.name,
[formattedTransactionId, requestDetails],
[formattedTransactionId],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking is this the only place to remove it?
Other approach is to set IP undefined after it's utilized in the rate limiter if possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, its not the only place, i think there is one more, working on it
I will check your suggestion as well

@konstantinabl konstantinabl force-pushed the 3266-relay-is-logging-ip-addresses branch from d5db1d0 to e0a7de0 Compare November 15, 2024 14:30
@quiet-node quiet-node marked this pull request as draft November 15, 2024 15:04
@konstantinabl konstantinabl force-pushed the 3266-relay-is-logging-ip-addresses branch from e0a7de0 to 9302c27 Compare November 15, 2024 15:10
@konstantinabl konstantinabl force-pushed the 3266-relay-is-logging-ip-addresses branch 2 times, most recently from d4ff0f7 to 9e307bf Compare November 15, 2024 15:15
@konstantinabl konstantinabl force-pushed the 3266-relay-is-logging-ip-addresses branch from 9e307bf to 7e24f26 Compare November 15, 2024 15:17
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 15, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@konstantinabl konstantinabl marked this pull request as ready for review November 15, 2024 15:33
@konstantinabl konstantinabl changed the title fix: removes request details from args so ip addresses are not logged fix: masks ip address in request details passed as argument to MN client Nov 15, 2024
@codecov Codecov
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.89%. Comparing base (b99655e) to head (7e24f26).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3268      +/-   ##
==========================================
+ Coverage   76.64%   77.89%   +1.25%     
==========================================
  Files          65       66       +1     
  Lines        4449     4470      +21     
  Branches     1014     1003      -11     
==========================================
+ Hits         3410     3482      +72     
+ Misses        672      613      -59     
- Partials      367      375       +8
Flag Coverage Δ
config-service 98.14% <ø> (ø)
relay 78.64% <100.00%> (+0.06%) ⬆️
server 83.28% <ø> (?)
ws-server 36.66% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/relay/src/lib/clients/mirrorNodeClient.ts 86.19% <100.00%> (-1.54%) ⬇️
packages/relay/src/lib/eth.ts 71.07% <100.00%> (-3.55%) ⬇️

... and 29 files with indirect coverage changes

quiet-node
quiet-node approved these changes Nov 15, 2024
View reviewed changes
Copy link
Member

@quiet-node quiet-node left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for the quick great work!

Nana-EC
Nana-EC reviewed Nov 16, 2024
View reviewed changes
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good but I won't if there is a single location we can set this oce

packages/relay/src/lib/eth.ts
@@ -1720,9 +1721,16 @@ export class EthImpl implements Eth {
if (submittedTransactionId) {
try {
const formattedTransactionId = formatTransactionIdWithoutQueryParams(submittedTransactionId);

// Create a modified copy of requestDetails
const modifiedRequestDetails = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works but is there a place where we can do this once before it's passed into the eth.ts but after it's utilized locally?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nana-EC Nana-EC Nana-EC left review comments

@quiet-node quiet-node quiet-node approved these changes

@AlfredoG87 AlfredoG87 Awaiting requested review from AlfredoG87 AlfredoG87 is a code owner

@ebadiere ebadiere Awaiting requested review from ebadiere

@lukelee-sl lukelee-sl Awaiting requested review from lukelee-sl lukelee-sl is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@konstantinabl konstantinabl

Labels
Internal For changes that affect the project's internal workings but not its outward-facing functionality.
Projects
Smart Contract Sprint Board
Status: Backlog
Milestone
0.61.0
Development

Successfully merging this pull request may close these issues.

Relay is logging IP addresses
3 participants
@konstantinabl @Nana-EC @quiet-node